Add RLC-only SideEffectFree stubs and tests#7609
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Log4j stubs and AutoCloseable/Closeable stubs, updates the Suggested reviewers
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astub`:
- Around line 24-33: The import of java.io.IOException is redundant because this
stub is already in package java.io; remove the line importing
java.io.IOException from the file containing the Closeable interface (the file
declaring interface Closeable and method close(`@GuardSatisfied` Closeable this)
throws IOException) so the code relies on the package-visible IOException;
verify no other external imports are required and run cleanup to ensure no
unused-import warnings remain.
In `@checker/tests/resourceleak/sideeffect-free-stubs/AutoCloseableClose.java`:
- Around line 18-29: The test AutoCloseableClose.java has the same risky pattern
as CloseableClose.java: in the close() method (annotated with
`@EnsuresCalledMethods`(value={"this.first","this.second"}, methods="close")) a
call to first.close() can throw and prevent second.close(), violating the
exceptional postcondition; update the test by either adding the expected error
annotation (e.g. "// :: error: contracts.exceptional.postcondition") on the
method or before the close() implementation to mirror CloseableClose.java, or
add a short comment in the file explaining why this case is intentionally
different and does not produce the same error, referencing the method close(),
and the fields first and second so the intent is clear.
In `@checker/tests/resourceleak/sideeffect-free-stubs/CloseableClose.java`:
- Around line 24-27: Update the error annotation on the close() method to use
the canonical square-bracket format: change the comment token `// :: error:
contracts.exceptional.postcondition` to `// :: error:
[contracts.exceptional.postcondition]` so it matches the pattern used in
TwoResourcesECM.java and other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21144dd9-af2c-46f9-8381-edb29a8bb8f6
📒 Files selected for processing (8)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsChecker.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astubchecker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astubchecker/tests/resourceleak/TwoResourcesECM.javachecker/tests/resourceleak/sideeffect-free-stubs/AutoCloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs/CloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4jObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/ThrowablePrintStackTrace.java
💤 Files with no reviewable changes (1)
- checker/tests/resourceleak/TwoResourcesECM.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub`:
- Around line 1-19: The stubs only declare debug/warn/error but miss other
common side-effect-free logging methods, causing inconsistent checker behavior;
update the org.apache.log4j.Logger class and org.apache.logging.log4j.Logger
interface to also declare `@SideEffectFree` methods info(Object), trace(Object),
fatal(Object) and the corresponding overloads info(Object, Throwable),
trace(Object, Throwable), fatal(Object, Throwable) so all common log-level
methods (both single-arg and Object+Throwable overloads) are treated as
side-effect-free by the checker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c77b8439-87a9-452f-b884-47092b18daf2
📒 Files selected for processing (5)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astubchecker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astubchecker/tests/resourceleak/sideeffect-free-stubs/CloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub`:
- Around line 35-36: The stub methods like trace(Message message) and
trace(Message message, Throwable throwable) (and the corresponding overloads for
debug, info, warn, error, fatal) use an unqualified Message type which doesn't
resolve; update each parameter type from Message to the fully-qualified
org.apache.logging.log4j.message.Message in those method signatures (e.g.,
trace, debug, info, warn, error, fatal overloads) so the stub binds correctly
and `@SideEffectFree` can be applied.
In `@checker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.java`:
- Around line 12-22: Add a regression test that exercises the Logger
throwable-overload to cover the stubbed (…, Throwable) mapping: call the
error(String, Throwable) overload (for example error("after close", new
RuntimeException())) in the same test file/class where object and varargs paths
are exercised (Log4j2ObjectCalls and the related tests around lines 54–112) so
the throwable-specific stub/mapping is invoked and protected from regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c35a067-4d20-45a5-a09e-c3266c2b256d
📒 Files selected for processing (3)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astubchecker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.java
kelloggm
left a comment
There was a problem hiding this comment.
I have some concerns about the test case structure.
I am also not certain that these stubs need to be RLC-specific (at least, the logging ones). I understand that these functions aren't literally side-effect-free in the technical sense (because writing to the log is a side-effect), but for any checker that's not reasoning about logging they might as well be, no? For example, I don't see why we wouldn't want these stubs for the Nullness Checker, too.
@mernst do you have an opinion on this question? That is, should we add @SideEffectFree stubs for logging functions for checkers besides the RLC, because they will help avoid false positives, even though the logging functions aren't technically side-effect free - but they are side-effect-free for the purposes of the analysis?
I saw there is existing log4j stubs for the nullness checker too ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub`:
- Around line 6-25: The new astub adds extra untested overload families
(Category.log(...), trace(...), fatal(...), and Log4j2 Message overloads) so
regressions or binding typos there won't be caught; update the tests to exercise
these newly stubbed signatures (or add new test cases similar to
checker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.java and
Log4j2ObjectCalls.java) to cover Category.log(Priority,...),
Category.log(String,Priority,...), Logger.trace(...), Category.fatal(...), and
any Message-based overloads so the checker validates the bindings for those
methods as well. Ensure the tests include both object and throwable/varargs
paths for each added overload family to detect binding mistakes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f772ae88-e239-443a-b0a1-2e41d531ef46
📒 Files selected for processing (4)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astubchecker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/ThrowablePrintStackTrace.java
Based on the email conversation with @mernst and @kelloggm, @mernst suggested that it's fine to treat the logging methods as side-effect-free for all checkers. So I've made the following changes:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astub`:
- Around line 16-19: Remove the redundant access modifier from the
Closeable.close declaration: in the interface Closeable (the method annotated
with `@SideEffectFree` and taking `@GuardSatisfied` Closeable this and throwing
IOException) drop the explicit "public" so the method matches the
implicit-public style used by AutoCloseable.close for consistency.
In
`@checker/tests/resourceleak/sideeffect-free-stubs-tests/ClosableResource.java`:
- Around line 1-14: The header comment and one helper class are misspelled:
rename the file to CloseableResource.java, correct the header comment to
"Closeable and AutoCloseable", and rename class AutoClosableResource to
AutoCloseableResource (keeping CloseableResource as-is); then update all
references/usages of AutoClosableResource (e.g., in AutoCloseableClose and any
other tests) to the new class name so compilation succeeds.
In
`@checker/tests/resourceleak/sideeffect-free-stubs-tests/Log4j2ObjectCalls.java`:
- Around line 11-81: Consolidate the six near-identical test classes
(Log4j2DebugObject, Log4j2DebugVarargs, Log4j2InfoObject, Log4j2WarnObject,
Log4j2ErrorObject, Log4j2ErrorWithThrowable) into a single test class (e.g.,
Log4j2ObjectCalls) that holds the `@Owning` CloseableResource field and exposes
multiple close-like methods (e.g., closeDebug(), closeDebugVarargs(),
closeInfo(), closeWarn(), closeError(), closeErrorWithThrowable()) each
annotated with `@EnsuresCalledMethods`(value = "this.resource", methods = "close")
and calling resource.close() followed by the respective logger call; this keeps
coverage for each logging variant while removing duplicate classes and
preserving the same logger usage and annotations.
- Around line 1-3: Update the file header comment to correct the broken sentence
and reflect that the Log4j stubs are no longer RLC-specific: rewrite the first
two lines so they read clearly (e.g., "This test covers the Log4j 2.x API in
package org.apache.logging.log4j; it uses the real library API.") and change the
second sentence to state that the Log4j stubs now live under
framework/src/main/java/org/checkerframework/framework/stub/jdk.astub and
therefore apply to all checkers (remove any mention of "RLC-specific"). Ensure
the new comment is concise and accurate and replace the existing erroneous
lines.
In `@framework/src/main/java/org/checkerframework/framework/stub/jdk.astub`:
- Around line 24-27: The Logger class members are indented with 4 spaces while
Category members use 2 spaces; update the indentation of the Logger member
declarations in class Logger (the two `@SideEffectFree` trace(...) lines) to use
2-space indentation to match Category for consistent formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21809b66-ad8a-4eaa-a4f8-ba5778fbffd2
📒 Files selected for processing (9)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsChecker.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astubchecker/tests/resourceleak/sideeffect-free-stubs-tests/AutoCloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs-tests/ClosableResource.javachecker/tests/resourceleak/sideeffect-free-stubs-tests/CloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs-tests/Log4j1ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs-tests/Log4j2ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs-tests/ThrowablePrintStackTrace.javaframework/src/main/java/org/checkerframework/framework/stub/jdk.astub
Moving the log4j stubs to |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/tests/resourceleak/sideeffect-free-stubs-tests/CloseableResource.java`:
- Around line 1-2: Fix the typos in the file header comment for the
CloseableResource class: change "reources" to "resources" and "Autocloseable" to
"AutoCloseable" so the comment correctly reads that the class represents
resources that implement Closeable and AutoCloseable for use by other tests.
In `@framework/src/main/java/org/checkerframework/framework/stub/jdk.astub`:
- Around line 1-6: Update the header comment in
framework/src/main/java/org/checkerframework/framework/stub/jdk.astub to explain
why this non-JDK content is named jdk.astub and loaded globally: state that
AnnotationFileElementTypes.parseStubFiles() auto-discovers this file for all
checkers (the mechanism used to achieve framework-level global loading without
per-checker wiring). Also document the semantic limitation: that methods (e.g.,
Log4j APIs in package org.apache.log4j) are annotated `@SideEffectFree` for
practical flow-refinement reasons but this is a pragmatic/lenient choice because
logging produces external I/O; advise maintainers they can override in
checker-specific stubs if stricter I/O/purity semantics are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 738c0a80-4ecc-4cce-9bc1-fab3e9d714f8
📒 Files selected for processing (3)
checker/tests/resourceleak/sideeffect-free-stubs-tests/AutoCloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs-tests/CloseableResource.javaframework/src/main/java/org/checkerframework/framework/stub/jdk.astub
|
@iamsanjaymalakar CI is failing because log4j isn't always on the classpath, which is causing the stub parser to emit extra warnings. The best fix would be to make loading the stub conditional on log4j being on the classpath, but I'm not sure if we have the capability to do that; @mernst would know. If not, you should look into how other library stubs are loaded - I know that we have annotations for e.g., guava, but I think we use our own forks (i.e., typetools/guava) for them rather than storing them in stub files. |
@kelloggm keeping the log4j stubs for RLC only. To load them like other library stubs I'd need to create own forks and annotate them which is larger in scope than this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub`:
- Around line 1-5: Add a short header comment to the top of the log4j.astub file
documenting that the `@SideEffectFree` annotations are a deliberate, RLC-only
compromise (logging performs I/O) and that this stub is intentionally loaded
only via RLCCalledMethodsChecker.@StubFiles (not as a framework-level stub) to
avoid accidental removal or promotion; mention the rationale from the discussion
with `@mernst` / `@kelloggm` so future maintainers understand why the annotation and
restricted scope are required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d15499f3-5715-4c52-af56-c7665c2ae1cc
📒 Files selected for processing (2)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsChecker.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub
Ok. This feels like a suboptimal outcome for the CF as a whole, but I agree it's an improvement to the RLC and I won't let the perfect be the enemy of the good here. |
Add targeted RLC-only
@SideEffectFreestubs so RLC preserves previously established Called Methods facts across a few non-mutating helper calls. For example:The
printStackTrace()call should not cause RLC to forget thatrwas already closed.This adds stub support for
AutoCloseable.close(),Closeable.close(),Throwable.printStackTrace(...), and Log4jdebug(Object),warn(Object), anderror(Object), along with regression tests.